Skip to content

Conversation

@ljbade
Copy link

@ljbade ljbade commented Mar 30, 2022

The Summary metric type uses an internal std::chrono::steady_clock to track when to update the metric buckets.

This PR changes it to use system_clock and also adds functions to allow a custom time source to be passed into the Observe() and Collect() functions.

This will allow summary metrics to be correctly calculated when replaying data.

@ljbade ljbade requested review from notoriaga and silverjam March 30, 2022 09:07
@ljbade
Copy link
Author

ljbade commented Mar 30, 2022

I still need to add test cases, but wanted to run it through CI to take a look at the coverage.

@coveralls
Copy link

coveralls commented Mar 30, 2022

Pull Request Test Coverage Report for Build 2113568677

  • 28 of 28 (100.0%) changed or added relevant lines in 6 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.06%) to 96.446%

Totals Coverage Status
Change from base Build 2074426956: 0.06%
Covered Lines: 787
Relevant Lines: 816

💛 - Coveralls

void ObserveMultiple(const std::vector<double>& bucket_increments,
const double sum_of_values);

/// \brief Get the current value of the counter.
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just made upstream PR to fix this, so will remove from this commit.

@ljbade ljbade force-pushed the ljbade/summary-time-2 branch from fe1bcbc to 2a95ef7 Compare March 30, 2022 22:37
Base automatically changed from ljbade/om to master April 1, 2022 00:51
@silverjam
Copy link

@ljbade looks like this has some conflicts with master?

Copy link

@silverjam silverjam left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@ljbade
Copy link
Author

ljbade commented Apr 8, 2022

yeah I need to rebase

@ljbade ljbade force-pushed the ljbade/summary-time-2 branch from 2a95ef7 to e6f1992 Compare April 8, 2022 06:48
@ljbade ljbade merged commit e294939 into master Apr 14, 2022
@ljbade ljbade deleted the ljbade/summary-time-2 branch April 14, 2022 03:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants